Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update roles with interactive info #2517

Closed
wants to merge 3 commits into from
Closed

Conversation

straker
Copy link
Contributor

@straker straker commented Sep 15, 2020

The information was taken from https://www.w3.org/TR/html-aria/#allowed-aria-roles-states-and-properties

This is a precursor to #601. From reading the discussion in that issue, there was a lot of concern with false positives of nested roles. For example, a menuitem cannot have interactive descendants but is allowed a menu which could have a menuitem.

To address that, I believe the following algorithm works in all listed cases of that pr:

  1. find all interactive roles which have interactiveDescendant: false. An interactive role is defined as a role whose contentTypes includes interactive (this step can be done in a rule matcher)
  2. loop through all descendants
    1. if the descendants HTML or ARIA role contentTypes includes flow, continue to the next descendant
      1. if the descendants HTML or ARIA role contentTypes also includes interactive then don't loop through its descendants as the algorithm will start again from that descendant
    2. else if the descendants HTML or ARIA role contentTypes includes interactive it fails

This should catch all the issues brought up in the original issue, allowing menuitems to have menu children as it is both flow and interactive (causing the algorithm to break and start again from there). It should also catch ARIA buttons having nested HTML interactive elements such as anchors or buttons.

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@straker straker requested a review from a team as a code owner September 15, 2020 16:20
Comment on lines -5 to +8
<div class="foo" id="foo">foo</div>
<div role="button">
Hello
<div role="button">Goodbye</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you didn't mean to put this in?

- `heading`
- `phrasing`
- `interactive`
- `interactiveDescendant` - boolean(optional. Default `true`). If the role is allowed to have interactive descendants.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is exclusively based on whether contentType includes "interactive", this is redundant. I'm not sure this is the right solution though. What I was thinking is to look at ARIA's "presentational children" property. The issue isn't that screen readers just don't know how to deal with nested interactive elements. It's that anything you put inside of an element with presentational children will not be included in the accessibility tree. Not just interactive stuff either:

The image in this button is not in the accessibility tree. This button wouldn't be announced as having an image inside it. But if you put role=link on the button, it will be.

<button><img src="..." alt="hello" /></button>

There are interactive elements that require other interactive children. Combobox and listbox jump to mine. I suspect you can also do it with links without causing accessibility problems. ARIA definitely says you can, but we should probably test that to be sure.

@straker
Copy link
Contributor Author

straker commented Sep 30, 2020

Closing per Wilcos comment (making a new PR with presentational children instead)

@straker straker closed this Sep 30, 2020
@WilcoFiers WilcoFiers deleted the interactive-roles branch January 30, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants